Skip to content

Change initial_point default strategy from "prior" to "moment"#5140

Merged
ricardoV94 merged 6 commits intopymc-devs:mainfrom
ricardoV94:check_distributions_moments_coverage
Dec 13, 2021
Merged

Change initial_point default strategy from "prior" to "moment"#5140
ricardoV94 merged 6 commits intopymc-devs:mainfrom
ricardoV94:check_distributions_moments_coverage

Conversation

@ricardoV94
Copy link
Copy Markdown
Member

@ricardoV94 ricardoV94 commented Nov 5, 2021 •
edited
Loading

This PR adds a check that we have moments for all distributions and changes the default initial point strategy from "prior" to "moment"

Also added a check in the default test moments that the logp of the provided moment is finite.

Closes #5009
Related to #5078

We also need to decide whether we default to a draw from the prior if a distribution does not have a get_moment implemented or if we just fail in that case (which is what is happening now). It would be trivial to allow this with a try/except block

@ricardoV94 ricardoV94 added this to the v4.0.0-beta1 (vNext) milestone Nov 5, 2021
@ricardoV94 ricardoV94 force-pushed the check_distributions_moments_coverage branch from 3fb0e1a to 88cce65 Compare November 5, 2021 10:56
@codecov
Copy link
Copy Markdown

codecov bot commented Nov 5, 2021 •
edited
Loading

Codecov Report

Merging #5140 (c6e11eb) into main (0c90e82) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5140      +/-   ##
==========================================
+ Coverage   79.15%   79.17%   +0.01%     
==========================================
  Files          88       88              
  Lines       14311    14316       +5     
==========================================
+ Hits        11328    11334       +6     
+ Misses       2983     2982       -1
Impacted Files Coverage Δ
pymc/distributions/continuous.py 97.37% <ø> (+0.50%) ⬆️
pymc/initial_point.py 100.00% <100.00%> (ø)
pymc/backends/report.py 89.51% <0.00%> (-2.10%) ⬇️
pymc/parallel_sampling.py 86.33% <0.00%> (-1.00%) ⬇️
pymc/distributions/discrete.py 98.72% <0.00%> (+0.25%) ⬆️
pymc/distributions/distribution.py 95.23% <0.00%> (+0.52%) ⬆️

@ricardoV94 ricardoV94 force-pushed the check_distributions_moments_coverage branch from 88cce65 to d17d0b2 Compare November 17, 2021 10:06
@ricardoV94 ricardoV94 changed the title Add test_all_distributions_have_moments Add test to ensure all distributions have moments Nov 18, 2021
@ricardoV94 ricardoV94 force-pushed the check_distributions_moments_coverage branch from d17d0b2 to b8baccf Compare November 19, 2021 11:15
@ricardoV94 ricardoV94 changed the title Add test to ensure all distributions have moments Change initial_point default strategy from "prior" to "moment" Nov 19, 2021
@ricardoV94 ricardoV94 mentioned this pull request Nov 19, 2021
Closed
@ricardoV94
Copy link
Copy Markdown
Member Author

ricardoV94 commented Nov 19, 2021

Getting some weird parallel sampling test failures only on Windows. Perhaps related to #4904?

@ricardoV94 ricardoV94 force-pushed the check_distributions_moments_coverage branch 2 times, most recently from a1fb23e to b97a157 Compare November 19, 2021 13:12
@ricardoV94
Copy link
Copy Markdown
Member Author

ricardoV94 commented Nov 20, 2021

I am inclined to defaulting back to a prior sample if a distribution does not have a moment, perhaps with a UserWarning like

Moment not defined for distribution X, defaulting to a draw from the prior. This can lead to difficulties during tuning. You can manually define an initval or implement a get_moment dispatched function for this distribution.

This would allow users to more gradually implement their own distributions. We still have a check added in this PR that all our standard distributions have moments implemented.

@michaelosthege
Copy link
Copy Markdown
Member

michaelosthege commented Nov 20, 2021

I like the fallback plus warning idea. Maybe include a link to the relevant docs section on implementing custom distributions?

@ricardoV94 ricardoV94 force-pushed the check_distributions_moments_coverage branch 2 times, most recently from 895b44c to 15e3632 Compare November 25, 2021 14:57
@ricardoV94 ricardoV94 force-pushed the check_distributions_moments_coverage branch from 15e3632 to efe1b88 Compare December 6, 2021 18:10
@ricardoV94 ricardoV94 marked this pull request as ready for review December 6, 2021 18:10
@ricardoV94
Copy link
Copy Markdown
Member Author

ricardoV94 commented Dec 6, 2021

We have very few moments missing, and it was consensus that this should no longer block the beta release.

I have added the known missing moments to test_all_distributions_have_moments. This test will fail whenever those moments get implemented, making sure our coverage stays up to date.

@ricardoV94 ricardoV94 force-pushed the check_distributions_moments_coverage branch from efe1b88 to c6268c5 Compare December 6, 2021 19:17
@ricardoV94
Copy link
Copy Markdown
Member Author

ricardoV94 commented Dec 6, 2021

Still have those surprising failing tests on Windows alone...

@michaelosthege
Copy link
Copy Markdown
Member

michaelosthege commented Dec 7, 2021

Still have those surprising failing tests on Windows alone...

To me it looks like the same problem as #4904 - broadcasts are not correctly propagated through Cast operations.

@ricardoV94
Copy link
Copy Markdown
Member Author

ricardoV94 commented Dec 8, 2021 •
edited
Loading

Windows issues are related to a mix of:

  1. RandomVariables have unnecessary SpecifyShape when size is casted aesara-devs/aesara#691,
  2. Wrong broadcastable output of Alloc when using SpecifyShape aesara-devs/aesara#692

@ricardoV94 ricardoV94 mentioned this pull request Dec 8, 2021
Open
@ricardoV94 ricardoV94 force-pushed the check_distributions_moments_coverage branch 2 times, most recently from a557fc9 to dc47cf1 Compare December 12, 2021 15:14
@ricardoV94
Copy link
Copy Markdown
Member Author

ricardoV94 commented Dec 12, 2021

Tests are passing with the latest Aesara version

michaelosthege
michaelosthege reviewed Dec 12, 2021
View reviewed changes
@ricardoV94 ricardoV94 force-pushed the check_distributions_moments_coverage branch from dc47cf1 to c6e11eb Compare December 13, 2021 08:45
@ricardoV94 ricardoV94 merged commit e5fb0e7 into pymc-devs:main Dec 13, 2021
@ricardoV94 ricardoV94 deleted the check_distributions_moments_coverage branch January 3, 2022 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michaelosthege michaelosthege michaelosthege approved these changes

Assignees

No one assigned

Labels

v4

Projects

None yet

Milestone

v4.0.0b1 (vNext)

Development

Successfully merging this pull request may close these issues.

Change default initial_point method to "moment"

2 participants

@ricardoV94 @michaelosthege